Review — Multi-profile authentication support

Overview

Replaces the plain-text ~/.screenly single-token file with a YAML profile store, adding login --name, logout --name, auth list, auth switch, and me commands. Plain-text legacy files are transparently migrated. The migration design (auto-upgrade plain text → default profile on first write) is well thought out, and there's solid unit-test coverage for Authentication state transitions.

Correctness

• Token file permissions are not restricted. The YAML file now holds multiple tokens but is still written via fs::write with default umask. The old behavior had the same flaw, but the blast radius is larger now (compromise of one file = all profiles). Consider fs::set_permissions(&path, Permissions::from_mode(0o600)) on Unix after write_store (src/authentication.rs write_store).

• remove_token post-delete active selection is non-deterministic. When removing the active profile, store.active = store.tokens.keys().next().cloned() picks an arbitrary HashMap key — different on each run. Sort or pick deterministically (e.g., alphabetical first), and info!() the new active profile name so the user knows what just happened.

• Logout panics on any error via .expect(\"Failed to remove token.\") in cli.rs. Pre-existing pattern, but the new remove_token has more failure modes (NoCredentials, ProfileNotFound, Yaml parse). Better to print a friendly message and exit(1), matching other auth paths.

• fetch_profile_info assumes /v4.1/users/me returns an array. user.get(0).and_then(|u| u[\"email\"].as_str()) only works if the response is [{...}], not {...}. The tests mock both shapes consistently, but please confirm the live API actually returns an array — if it returns an object, me will silently print \"unknown\".

• No User-Agent header on profile-info requests. Existing Authentication::client() sets screenly-cli {version}; fetch_profile_info builds a bare client. Minor, but inconsistent.

• Race on store updates. read → mutate → write in verify_and_store_token / remove_token / switch_profile isn't atomic; two concurrent screenly login invocations can lose writes. CLI use makes this unlikely, but a temp-file + rename would be cheap insurance.

Style / Conventions

• Duplicated table-rendering block in AuthCommands::List and AuthCommands::Switch { name: None } — ~25 identical lines. Extract a print_profiles_table(profiles, api_url) helper.

• AuthCommands::Switch with no name printing the list is undocumented. The clap help reads <NAME> — Profile name to activate, with no hint that omitting it is a meaningful action. Either document it (/// Without an argument, prints the profile list.) or make name required.

• Me "Profile:" line shown conditionally based on active_profile_name(). When API_TOKEN env var is in use, the field is silently omitted in both human and JSON output. Reasonable, but consider printing Profile: (from API_TOKEN env) so the user understands why no profile name appears.

• Redundant use serde_yaml; at the top of src/authentication.rs — the serde_yaml::Error reference in the enum already pulls in the crate path.

• #[error(\"yaml error\")] swallows the underlying message. Use #[error(\"yaml error: {0}\")] so parse failures are diagnosable.

Test coverage

Good additions for migration, switch, list, and fetch_profile_info. Missing:

• login requires --name when multiple profiles exist — the new safety check is the headline UX claim of the PR and isn't covered.
• verify_and_store_token preserves existing profiles when called with a new name — easy to regress if someone refactors to TokenStore::default().
• remove_token(Some(name)) with explicit name — only the None (active) path is tested.
• Remove-active-profile picks a remaining one as new active — would also catch the non-determinism above.

Performance

• auth list and auth switch (no arg) make 2 sequential HTTP requests per profile (users/me + teams). For 5+ profiles this is a noticeable hang. Consider parallelizing with futures::join_all (already a dep), or calling a combined endpoint if available.

Security

• See file-permissions point above — primary concern.
• Tokens are passed around in (name, token, is_active) tuples returned from list_profiles(). Internal-only, but token strings are unused in the caller — consider returning (name, is_active) and exposing token lookup via a separate method to reduce accidental-print risk.

Summary

Solid feature with a clean migration path. Top priorities before merge: deterministic active-profile selection on removal, file permissions (0o600), graceful logout error handling, and tests for the --name enforcement and explicit-name removal. The duplicate table code and undocumented switch no-arg behavior are easy follow-ups.